Dependency Review — tts-voiceover skill

Summary

This PR adds a new tts-voiceover skill with 4 runtime dependencies and a uv.lock file pinning all transitive dependencies.

Dev/fuzz dependencies (pytest, ruff, atheris) follow the established repo pattern and are consistent with other skills.

1. New Dependencies

azure-cognitiveservices-speech v1.49.0 — Microsoft's Azure Cognitive Services Speech SDK. MIT licensed, actively maintained by Microsoft, uploaded 2026-04-07. Necessary for the core TTS functionality; no equivalent exists in the project. ✅

azure-identity — Microsoft's Azure Identity library for Entra ID token auth. MIT licensed, well-maintained. Necessary for the SPEECH_RESOURCE_ID auth path; no duplicate in the project. ✅

python-pptx and pyyaml — Both already present in the sibling powerpoint skill. Licenses confirmed MIT. Each skill carries its own pyproject.toml/lock file per repo convention, so this duplication is expected and correct. ✅

2. Version Specifiers

Runtime dependencies in pyproject.toml are unpinned (no minimum version constraints). This matches the pattern used in the powerpoint skill and other experimental skills in this repo. The uv.lock file provides actual reproducible pinning at install time, which is the uv-recommended approach. ✅

3. SHA Pinning Compliance

No GitHub Actions workflow files were modified. N/A. ✅

4. Devcontainer and Setup Alignment

No changes to .devcontainer/ or copilot-setup-steps.yml. N/A. ✅

Observations

• The dev group omits pytest-mock and pytest-cov that other skills include. This is not blocking, but worth noting if test coverage tooling is expected.
• azure-cognitiveservices-speech declares azure-core as a transitive dependency, which in turn pulls in requests, certifi, and charset-normalizer. All are permissively licensed (Apache-2.0 or MIT) and compatible with this project's MIT license.

All safety checks passed. No blocking issues found.

Generated by Dependabot PR Review for issue #1415 · ● 1.6M

Automated Quality Review — feat(skills): add tts-voiceover skill

This PR introduces a well-structured Python skill for Azure TTS voice-over generation. The code quality is generally high — copyright headers, exit codes, argparse/pathlib/logging patterns, type hints, and the fuzz harness all follow repo conventions. There are, however, three blocking issues that must be resolved before merge.

🔗 Issue Alignment

❌ No linked issue.

The "Related Issue(s)" section contains only None. All PRs must reference a linked issue using Fixes #, Closes #, or Resolves #. Please open an issue describing the motivation for this skill and link it here.

📋 PR Template Compliance

❌ Required automated checks not completed.

The following items in the Required Automated Checks section remain unchecked with no indication they were run or are N/A:

npm run lint:ps is N/A here (no PowerShell files changed) and can be annotated accordingly.

❌ AI Artifact Contributions checklist items unchecked.

Used /prompt-analyze to review contribution and Addressed all feedback from prompt-builder review are both unchecked. These require manual verification by the author.

🛠️ Coding Standards

Python scripts comply with repo conventions (python-script.instructions.md, python-test.instructions.md): copyright headers ✅, exit codes ✅, create_parser() ✅, pathlib ✅, structured logging ✅, if __name__ == "__main__" guard ✅, Google-style docstrings ✅. The fuzz harness follows the polyglot pattern with pytest fallback ✅. The pyproject.toml contains all required sections ([tool.ruff], [tool.ruff.lint], [tool.pytest.ini_options], fuzz group) ✅.

🔍 Code Quality

❌ [Blocking] SSML XML injection via unescaped speaker notes — inline comment on generate_voiceover.py line 90.

Speaker notes placed into the SSML document are not XML-escaped. Any note containing &, <, or > produces malformed XML that Azure TTS will reject. Fix by calling xml.sax.saxutils.escape(notes) before apply_acronym_aliases. See inline comment for the one-line fix.

❌ [Blocking] Non-existent file referenced in hve-core-all.collection.yml — inline comment on line 506.

.github/instructions/security-planning/mcp-security.instructions.md does not exist in the repository and was not added by this PR. This is unrelated scope creep that will cause npm run lint:collections-metadata to fail. Remove the two added lines or submit them in a separate PR along with the actual file.

⚠️ [Non-blocking] Entra ID token not refreshed for long-running decks — inline comment on generate_voiceover.py line 214.

The token is fetched once and reused for all slides. Tokens expire after ~1 hour, which may cause mid-run 401 failures on large decks. A refresh-before-expiry pattern is suggested in the inline comment.

i️ [Informational] Partial substring matching in apply_acronym_aliases.

str.replace() can match an acronym inside a longer word (e.g., AST inside FASTEST). The longest-first sort mitigates prefix overlaps but not embedded occurrences. Consider using \b-bounded regex for word-boundary matching when this matters for user-supplied content.

✅ Action Items

1. Open a tracking issue and add a Fixes # reference to the PR description.
2. Remove the mcp-security.instructions.md entry from collections/hve-core-all.collection.yml.
3. XML-escape notes before passing to apply_acronym_aliases (one-line fix at line 247).
4. Run and check npm run spell-check and npm run lint:md-links; update the checklist.
5. Address the unchecked AI Artifact Contributions items or explain why they are N/A.

Generated by PR Review for issue #1415 · ● 1.7M

PR Review

This PR adds a well-conceived tts-voiceover skill with solid overall structure — good SSML handling, dual auth support, a proper fuzz harness, and thorough documentation. Three issues need to be resolved before merging.

Issue Alignment

The linked issue (#1417) could not be read due to access restrictions. Based on the PR description alone, the core tts-voiceover skill additions appear well-scoped and purposeful. However, the addition of mcp-security.instructions.md to hve-core-all.collection.yml (see inline comment) introduces unrelated scope.

PR Template Compliance

Two checkbox gaps:

1. Script/automation (.ps1, .sh, .py) not checked — This PR adds two Python scripts (generate_voiceover.py, embed_audio.py). The "Script/automation" checkbox under Other in the Type of Change section should be checked.

2. New feature not checked — Adding a new skill is a non-breaking new feature. The "New feature" checkbox under Code & Documentation should also be checked.

3. Skill script requirement — The PR template states: "Skills: Must include both bash and PowerShell scripts." This skill ships Python-only scripts with no generate_voiceover.sh / generate_voiceover.ps1 wrappers. Cross-platform wrapper scripts (e.g., setting up the uv environment and invoking the Python entrypoints) are required to meet the stated skill contribution standard. See docs/contributing/skills.md.

Coding Standards

See inline comments for specifics. Summary:

• ⚠️ wrap_ssml XML injection (generate_voiceover.py lines 88–89): voice and rate CLI arguments are inserted into XML attribute positions without escaping. The PR correctly escapes speaker notes but misses these two attributes. Use xml.sax.saxutils.quoteattr() as shown in the inline comment.
• ❌ Unrelated/contradictory collection change (hve-core-all.collection.yml line 506): PR description claims mcp-security.instructions.md was removed, but the diff adds it. This entry is also missing a maturity field. Clarify and correct.
• 💡 Unpinned Azure SDK versions (pyproject.toml lines 6–9): Suggestion to add minimum version bounds for azure-cognitiveservices-speech and azure-identity (non-blocking, but recommended).

Code Quality

Beyond the issues above, the implementation is clean:

• Correct XML escaping of speaker notes before SSML substitution ✅
• Entra ID token refresh logic 5 min before expiry ✅
• longest-first acronym sort prevents partial-match shadowing ✅
• EXIT_SUCCESS/EXIT_FAILURE/EXIT_ERROR exit code constants ✅
• Proper from __future__ import annotations and modern type hints ✅
• Fuzz harness with Atheris/pytest polyglot mode ✅

Required Actions

1. Escape voice and rate in wrap_ssml using xml.sax.saxutils.quoteattr().
2. Resolve the mcp-security.instructions.md discrepancy in hve-core-all.collection.yml (remove if accidental; add maturity field and correct PR description if intentional).
3. Add bash and PowerShell wrapper scripts for the two Python entrypoints to satisfy the skill contribution requirement, or document why Python-only is acceptable for this skill.
4. Check the Script/automation and New feature checkboxes in the Type of Change section.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 1.6M

Automated PR Review — feat(skills): add tts-voiceover skill

Verdict: REQUEST_CHANGES — one security finding must be resolved before merge; three additional quality improvements are strongly recommended.

Issue Alignment

Linked issue Fixes #1417 is present. The diff is consistent with a new TTS skill addition. No obvious scope creep detected.

PR Template Compliance

⚠️ Unchecked required automated check: The checklist item for npm run lint:md-links is marked "deferred to CI" and left unchecked. Per the PR template instructions, this must be verified and checked before merge.

⚠️ "Script or automation" checkbox not checked: The diff includes six new scripts (.py, .ps1, .sh) but the "Script or automation" Type of Change checkbox is not selected. Please tick it.

⚠️ "Additional Notes" contradicts the diff: The notes section states that mcp-security.instructions.md was removed from hve-core-all.collection.yml. The actual diff shows no such removal in that file. The plugins/hve-core-all/README.md change adds mcp-security.instructions to the table. Please remove or correct this note.

Coding Standards

Applicable instruction files checked:

• python-script.instructions.md — Python conventions ✅ (modulo findings below)
• powershell.instructions.md — PowerShell conventions ⚠️ (duplication finding)
• bash.instructions.md — bash scripts ✅ clean
• markdown.instructions.md — SKILL.md ✅ clean; frontmatter valid
• prompt-builder.instructions.md — SKILL.md skill structure ✅ correct

Code Quality & Security

🔒 [BLOCKING] SSML attribute injection via alias values (generate_voiceover.py:76)
xml.sax.saxutils.escape(alias) does not encode ". If an acronyms.yaml alias value contains a double-quote, it escapes the alias="..." attribute in the generated SSML, enabling attribute injection or document structure corruption. Fix: xml.sax.saxutils.escape(alias, {'"': """}) or html.escape(alias, quote=True). See inline comment.

⚠️ Uninitialized variables referenced outside their conditional block (generate_voiceover.py:205)
speech_key, speech_region, and speech_resource_id are assigned only inside if not args.dry_run: but are referenced later in the loop body. The current guard prevents a NameError at runtime, but the code is fragile. Initialize to None before the conditional. See inline comment.

💡 Shared PowerShell helpers duplicated across two scripts (Invoke-EmbedAudio.ps1:56, Invoke-GenerateVoiceover.ps1:77)
Test-UvAvailability, Initialize-PythonEnvironment, and Get-VenvPythonPath are copy-pasted verbatim. Extract to scripts/Modules/TtsVoiceoverHelpers.psm1. See inline comment.

💡 Overly broad object type hint (embed_audio.py:39)
slide: object should be slide: pptx.slide.Slide for IDE support and type-checker correctness. See inline comment.

Action Items

1. [Required] Fix SSML alias attribute encoding — encode " as " in apply_acronym_aliases.
2. [Required] Check npm run lint:md-links and mark the checklist item.
3. [Required] Correct or remove the "Additional Notes" claim about mcp-security.instructions.md removal.
4. [Required] Check "Script or automation" in the Type of Change section.
5. [Recommended] Initialize speech_key, speech_region, speech_resource_id = None before the dry-run block.
6. [Recommended] Extract shared PowerShell helpers to a Modules/TtsVoiceoverHelpers.psm1.
7. [Recommended] Narrow slide: object to pptx.slide.Slide.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 3.3M

@auyidi1 wow, what a great PR.
I think when you cover the Copilot pr-review comments you are good to go

PR Review — feat(skills): add tts-voiceover skill

This PR adds a well-structured new skill with good cross-platform coverage (Python + Bash + PowerShell wrappers), a working fuzz harness, and solid documentation. The token-refresh logic and acronym escaping in apply_acronym_aliases are thoughtful. Two issues require changes before merge, and a few smaller items are noted below.

🔒 Issue Alignment

Linked issue #1417 could not be independently verified (integrity policy), but the PR description is coherent, the implementation matches the stated goals, and no obvious scope creep is present.

⚠️ PR Template Compliance

Required fix — "Script/automation" checkbox not checked:
The PR adds generate_voiceover.py, embed_audio.py, generate-voiceover.sh, embed-audio.sh, Invoke-GenerateVoiceover.ps1, and Invoke-EmbedAudio.ps1. The "Script/automation (.ps1, .sh, .py)" checkbox under Other must be checked to accurately reflect the type of change.

Minor — First AI Artifacts checkbox absent from Type of Change section:
The template's first AI Artifacts checkbox ("Reviewed contribution with prompt-builder agent and addressed all feedback") was dropped entirely rather than left unchecked with an N/A note. The Checklist section does explicitly N/A it, which is appreciated, but the Type of Change section should retain the unchecked item for completeness.

🔒 Security (blocking)

Incomplete SSML injection fix — see inline comment on generate_voiceover.py lines 89–90. The PR correctly escapes speaker_notes via xml.sax.saxutils.escape before SSML construction, but the voice and rate CLI arguments are interpolated into XML attributes verbatim. A value containing " would break the XML document structure. Escaping these two parameters completes the fix the PR description claims.

💡 Code Quality (non-blocking suggestions)

speechsdk potentially unbound in static analysis:
speechsdk is assigned inside if not args.dry_run: and later referenced in the token-refresh block inside the slide loop. At runtime this is safe because the refresh code is unreachable during a dry run. However, static analysis tools (mypy, Pyright) will flag the name as potentially unbound after a refactor. A forward declaration (speechsdk = None) before the conditional block, or moving the import to module level with a try/except ImportError, would make the intent explicit.

_make_entra_config return type is tuple not tuple[SpeechConfig, float]:
The bare tuple return annotation drops type precision. Per the python-script conventions, typed return annotations are preferred. Since speechsdk is a conditional import, a TYPE_CHECKING-guarded annotation or a comment-style annotation is acceptable.

generate_audio uses object for speech_config:
Same concern — object accepts any value and loses static type safety. A forward reference or a TYPE_CHECKING import of azure.cognitiveservices.speech.SpeechConfig would improve IDE support.

📋 Collection / Conventions (non-blocking)

maturity field missing — see inline comment on collections/experimental.collection.yml. New experimental-collection skills should carry an explicit maturity: experimental (or preview) rather than silently defaulting to stable. The same applies to the matching entry in hve-core-all.collection.yml.

✅ What looks good

• Cross-platform wrappers (Bash + PowerShell) following repo conventions; copyright headers, set -euo pipefail, main() pattern, and comment-based help are all present.
• pyproject.toml correctly includes [tool.ruff], [tool.pytest.ini_options], fuzz dependency group with atheris>=3.0, and python_files = ["test_*.py", "fuzz_harness.py"].
• Fuzz harness is a proper polyglot (pytest + Atheris) covering apply_acronym_aliases, wrap_ssml, and load_acronyms.
• Token refresh logic (5-minute pre-expiry window) for Entra ID long-running decks is a nice reliability touch.
• Acronyms sorted longest-first to prevent partial substring replacement — correct and well-considered.

✅ Required Actions

1. Complete the SSML escaping for voice and rate in wrap_ssml (see inline comment).
2. Check the "Script/automation" checkbox in the Type of Change section of the PR description.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2.3M

PR Review: feat(skills): add tts-voiceover skill

Overall the contribution is well-structured and shows careful attention to XML injection for speaker notes, cross-platform parity, and fuzz testing. However, there are a few required fixes before this can merge.

Issue Alignment

Linked issue #1417 could not be read due to repository access constraints. Based on the PR description, the implementation aligns with the stated goal of generating per-slide WAV voice-over files from YAML speaker notes using Azure Speech SDK.

No scope creep detected. The removal of the unrelated mcp-security.instructions.md reference from hve-core-all.collection.yml is a valid cleanup.

PR Template Compliance

⚠️ Missing Type of Change checkboxes — The PR adds .ps1, .sh, and .py files but only the Copilot skill box is checked. The Script/automation (.ps1, .sh, .py) checkbox under Other must also be checked to accurately reflect the change type.

Coding Standards Findings

Python (python-script.instructions.md)

1. generate_voiceover.py and embed_audio.py — Missing interrupt/pipe error handling (inline comments on both files): main() must catch KeyboardInterrupt (return 130) and BrokenPipeError (close stderr, return 1) at the top level per the instructions. The fix requires extracting the body into a run() helper.

PowerShell (powershell.instructions.md)

2. Invoke-GenerateVoiceover.ps1 and Invoke-EmbedAudio.ps1 — Missing main execution guard (inline comments on both files): The instructions require if ($MyInvocation.InvocationName -ne '.') wrapping the main execution block to enable dot-sourcing by Pester tests.

Security Findings

3. generate_voiceover.py wrap_ssml() — Incomplete SSML XML injection fix (inline comment): The PR description notes fixing SSML XML injection for speaker notes, and xml.sax.saxutils.escape() is correctly applied there. However, voice and rate CLI parameters are embedded directly into XML attributes without escaping. Use xml.sax.saxutils.quoteattr() on both for consistency with the existing fix.

Action Items

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2.1M

PR Review: feat(skills): add tts-voiceover skill

Overview

The new tts-voiceover skill is a well-structured addition — the Python scripts are organized clearly, the SKILL.md documentation is thorough, cross-platform wrappers are provided, and the fuzz harness is present. However, the review identified one security issue and several correctness / coding-standard violations that need to be addressed before merge.

🔒 Issue Alignment

Linked issue #1417 could not be fetched due to a repository access constraint. Review is based on the PR description, which presents a coherent narrative (generate TTS audio from slides, embed audio into PPTX). No obvious scope creep observed. If the issue contains acceptance criteria not reflected here, please verify them against the changes.

📋 PR Template Compliance

Action required: Check the Script or automation checkbox in the Type of Change section.

🔒 Security

SSML injection via unescaped --voice and --rate CLI arguments (generate_voiceover.py lines 88–89) — see inline comment.

voice and rate are CLI-supplied strings interpolated directly into XML attributes inside wrap_ssml(). Characters such as ", <, >, and & will produce malformed SSML. A value like en-US-Test&Debug generates invalid XML, and a crafted rate value could inject unexpected XML attributes. Both values must be passed through xml.sax.saxutils.escape(value, {'"': """}) before interpolation. Additionally, alias values in apply_acronym_aliases() (line 79) are substituted into attribute values and element text without escaping — the same xml.sax.saxutils.escape treatment is needed there.

🐛 Correctness

1. apply_acronym_aliases can corrupt emitted SSML tags (generate_voiceover.py line 79) — see inline comment. The iterative str.replace loop can match inside XML markup that was inserted by earlier iterations. Using a single-pass re.sub over all acronyms at once eliminates the problem.

2. embed_audio.py silently succeeds on per-slide failures (embed_audio.py lines 120–127) — see inline comment. The else branch logs the error but main() always returns EXIT_SUCCESS. Add a failed_count variable and return EXIT_FAILURE when any slide fails.

📋 Coding Standards

3. Missing KeyboardInterrupt / BrokenPipeError handling in main() for both generate_voiceover.py (line 185) and embed_audio.py (line 86) — see inline comment. Required by python-script.instructions.md.

4. Missing PowerShell main execution guard in both Invoke-EmbedAudio.ps1 (line 56) and Invoke-GenerateVoiceover.ps1 (line 77) — see inline comments. Required by powershell.instructions.md to support dot-sourcing for Pester tests.

5. Missing maturity: experimental in experimental.collection.yml (line 20) and hve-core-all.collection.yml (line 548) — see inline comments. Omitting the field causes both collection manifests to implicitly mark this experimental skill as stable. After adding the field, re-run npm run plugin:generate.

i️ Minor Observations (non-blocking)

• TtsVoiceoverHelpers.psm1: Export-ModuleMember uses comma-separated function names rather than the @('F1', 'F2', 'F3') array form preferred by powershell.instructions.md. Low priority.
• plugins/hve-core-all/README.md adds a mcp-security.instructions row. The PR description says this entry was removed from hve-core-all.collection.yml; the plugin README shows it being added. This looks like the collection YAML on main already contained the entry but the plugin outputs hadn't been regenerated until this PR. Not a blocking issue, but worth confirming intent.

✅ Action Items

1. Escape voice, rate, and all SSML-interpolated user-controlled strings with xml.sax.saxutils.escape.
2. Replace the iterative text.replace loop in apply_acronym_aliases with a single-pass re.sub.
3. Track failed_count in embed_audio.py and return EXIT_FAILURE when any slide fails.
4. Add KeyboardInterrupt / BrokenPipeError handling to main() in both Python scripts.
5. Wrap the main execution blocks in Invoke-EmbedAudio.ps1 and Invoke-GenerateVoiceover.ps1 with if ($MyInvocation.InvocationName -ne '.') { ... }.
6. Add maturity: experimental to both collection YAML entries and re-run npm run plugin:generate.
7. Check the Script or automation checkbox in the PR description's Type of Change section.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 3.5M

Review Summary

This PR adds a well-structured tts-voiceover skill with solid documentation, cross-platform wrappers, and a proper fuzz harness. The implementation covers Entra ID token refresh, SSML dry-run mode, and the content directory integration with the PowerPoint skill. Two security issues must be resolved before merge; the remaining findings are convention violations from the repository's coding standards.

🔒 Issue Alignment

Fixes #1417 — the PR implements the TTS voice-over generation feature described. The stated goals (WAV-per-slide, SSML acronym control, Entra ID auth, dry-run mode) are all present. No scope creep or missing requirements observed.

⚠️ PR Template Compliance

• Missing "Script/automation" checkbox — The PR adds .py, .sh, and .ps1 files but only the "Copilot skill" checkbox was checked under Type of Change. The Script/automation (.ps1, .sh, .py) checkbox should also be checked per the template.

🔒 Coding Standards — Security

💡 Coding Standards — Conventions

💡 Minor Observations (non-blocking)

• hve-core-all.collection.yml adds the skill without maturity: experimental. Per project conventions, omitting maturity defaults to stable. Given this is an experimental skill, consider adding maturity: experimental for accurate classification — consistent with other entries in the file that do declare their maturity.

✅ Strengths

• Fuzz harness correctly implements the polyglot pytest/Atheris pattern with corpus files and proper FUZZING guard.
• pyproject.toml follows all required sections ([tool.ruff], [tool.pytest.ini_options], fuzz group, python_files).
• xml.sax.saxutils.escape is applied to speaker notes text before SSML wrapping — good; just needs to be extended to the attribute values.
• Cross-platform wrappers (bash + PowerShell) are both present with matching feature parity.
• Entra ID token refresh logic (5-minute pre-expiry window) is correctly implemented.

🎯 Required Actions

1. Fix XML attribute injection in wrap_ssml — use xml.sax.saxutils.quoteattr() for voice and rate.
2. Add version lower bounds to all four production dependencies in pyproject.toml.
3. Check "Script/automation" in the PR Type of Change section.
4. Address the convention findings (PowerShell naming, invocation guard, module comments, Python error handling) as they are straightforward mechanical fixes.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2M

PR Review: feat(skills): add tts-voiceover skill

The implementation is well-structured overall — the Python scripts follow most conventions, the cross-platform wrapper pattern is solid, the fuzz harness is thorough, and the SSML-escaping of speaker notes is a good security call. However, several issues require resolution before merge.

🔗 Issue Alignment

The PR links Fixes #1417. The description and implementation are internally consistent and the scope appears appropriately bounded to a new skill. No scope creep detected.

📋 PR Template Compliance

⚠️ Missing checkbox — "Script/automation"

The PR adds .ps1, .sh, and .py scripts (7 files). The "Script/automation (.ps1, .sh, .py)" checkbox under the Other section of Type of Change must also be checked. The template says "Select all that apply."

All other template sections (description, related issue, sample prompts, testing, checklist) are filled correctly.

🧑💻 Coding Standards

Python (python-script.instructions.md)

PowerShell (powershell.instructions.md)

Skill file (prompt-builder.instructions.md)

🔒 Code Quality and Security

pytest-cov>=7.0 — unresolvable version constraint (pyproject.toml L17)

pytest-cov has not reached version 7.x. This constraint will fail uv sync for anyone setting up the development environment. Use >=5.0 or >=6.0.

SSML injection (generate_voiceover.py L88–89)

The voice and rate argument values are interpolated directly into the XML template. The PR already correctly XML-escapes speaker notes; the same xml.sax.saxutils.escape treatment must be applied to voice and rate. See the inline comment for the suggested fix.

maturity: experimental missing (hve-core-all.collection.yml L548)

Without the explicit field, the collection system defaults maturity to stable, which misclassifies a skill in the experimental track. See the inline comment.

✅ No Issues Found In

• Bash script conventions (shebang, copyright, set -euo pipefail, err(), usage(), quoting)
• pyproject.toml structure (ruff, pytest, fuzz group, python_files)
• Fuzz harness design and pytest mode compatibility
• apply_acronym_aliases longest-match ordering
• Entra ID token refresh logic (5-minute pre-expiry window)
• embed_audio.py functional correctness
• Collection YAML additions and plugin regeneration
• cspell dictionary update

📌 Required Actions Before Merge

1. Check the Script/automation checkbox in the PR description.
2. XML-escape voice and rate in wrap_ssml (see inline comment on generate_voiceover.py L88–89).
3. Fix pytest-cov>=7.0 → >=5.0 or >=6.0 in pyproject.toml.
4. Add maturity: experimental to the tts-voiceover entry in hve-core-all.collection.yml.
5. Remove the extra Copilot attribution line from SKILL.md (footer must be the last line).
6. Add the purpose comment header to TtsVoiceoverHelpers.psm1; switch Export-ModuleMember to array syntax.
7. Wrap main execution in the invocation guard in both Invoke-EmbedAudio.ps1 and Invoke-GenerateVoiceover.ps1.
8. Add KeyboardInterrupt / BrokenPipeError handling to main() in both Python scripts.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2.2M

Automated Quality Review — feat(skills): add tts-voiceover skill

The skill is well-structured and the overall implementation is clean. However, several blocking issues — security bugs, a correctness bug, and convention violations — need to be addressed before merge. The open review threads below summarise the required changes; each item links back to the thread where inline detail is captured.

🔒 Security

[Open thread] SSML attribute injection — voice and rate not XML-escaped in wrap_ssml
generate_voiceover.py lines 75–85: --voice and --rate CLI values are interpolated directly into XML attribute positions without xml.sax.saxutils.escape(). The PR description explicitly states "Fixed SSML XML injection", but the fix is incomplete — only speaker_notes were escaped. A value such as --voice 'foo"bar' produces malformed SSML and could be leveraged if this function is ever called with data from less-trusted sources.

[Open thread] SSML alias double-quoting — alias attribute not double-quote–escaped
apply_acronym_aliases escapes &, <, > via xml.sax.saxutils.escape() but the {'"': """} mapping needs to be passed consistently and verified against a lexicon entry that itself contains a double-quote character.

🐛 Correctness Bugs

[Open thread] text.replace() corrupts already-inserted SSML tags
After the first acronym replacement, the output string contains raw SSML markup (<sub alias="...">...</sub>). Subsequent text.replace() calls operate on this augmented string and can partially re-substitute text inside generated alias attribute values, producing malformed or nested SSML. A safe approach is to replace on the original text and track offsets, or use regex \b word-boundary matching against the pre-substituted string.

[Open thread] embed_audio.py::main() always returns EXIT_SUCCESS on embedding failure
When embed_slide_audio() returns False the error is logged but the function still returns EXIT_SUCCESS. Callers (including the bash/PowerShell wrappers) cannot detect partial failures; partially-narrated decks are silently saved.

📦 Collection / Metadata

[Open thread] Missing maturity: experimental on both collection entries
Both collections/experimental.collection.yml and collections/hve-core-all.collection.yml omit the maturity field. Per collection conventions, omitting it defaults to stable, which misrepresents a skill under experimental/. Both entries need maturity: experimental.

[New finding — inline comment] Unused lxml production dependency
pyproject.toml line 8 declares lxml>=6.1.0 but neither Python script imports it. This should be removed (or moved to a dev group if kept for future use). See the inline comment for details.

🔧 Convention Violations

[Open thread] Missing KeyboardInterrupt / BrokenPipeError handling — both Python scripts
Per python-script.instructions.md, main() must catch KeyboardInterrupt (exit 130) and BrokenPipeError at the top level. Neither generate_voiceover.py nor embed_audio.py does this.

[Open thread] Missing main execution guard — both PowerShell wrappers
Invoke-GenerateVoiceover.ps1 and Invoke-EmbedAudio.ps1 are missing the if ($MyInvocation.InvocationName -ne '.') { ... } guard required by powershell.instructions.md. Without it the scripts cannot be safely dot-sourced by Pester tests.

[Open thread] Conditionally assigned variables used outside their conditional block
speech_key, speech_region, and speech_resource_id in generate_voiceover.py are assigned only inside if not args.dry_run: but are referenced (for the Entra ID refresh check) in the loop body that runs regardless of execution path. Static analysers will flag these as potentially-undefined.

[Open thread] Unversioned production dependencies
All four runtime deps in pyproject.toml specify only a lower-bound (e.g. >=1.41). Combined with a uv.lock that is checked in, this means a regenerated lock file could pull breaking versions. Follow the existing skill pattern of constraining to a compatible range (e.g. >=1.41,<2).

📋 PR Template

[Open thread] "Script or automation" checkbox unchecked
The PR adds .py, .ps1, and .sh files matching the .*\.(ps1|sh|py)$ change-type pattern. The "Script/automation" checkbox under "Other" in the Type of Change section should be checked.

✅ Passing

• Frontmatter schema valid; name and description present.
• Copyright headers (# Copyright (c) Microsoft Corporation. + SPDX) present on all scripts.
• set -euo pipefail in both bash wrappers.
• Parallel bash/PowerShell wrappers provided as required by skill conventions.
• Fuzz harness covers apply_acronym_aliases, wrap_ssml, and load_acronyms with property-based tests.
• pyproject.toml includes required [tool.ruff], [tool.ruff.lint], [tool.pytest.ini_options], and python_files = ["test_*.py", "fuzz_harness.py"] sections.
• Collection YAML and plugin outputs updated.

Action items (must address before merge):

1. Escape voice and rate with xml.sax.saxutils.escape() in wrap_ssml.
2. Fix apply_acronym_aliases to avoid re-substituting already-generated SSML markup.
3. Return a non-zero exit code from embed_audio.py::main() when any embedding fails.
4. Add maturity: experimental to both collection entries.
5. Remove unused lxml from pyproject.toml production dependencies.
6. Add KeyboardInterrupt/BrokenPipeError handling to both Python main() functions.
7. Wrap #region Main in if ($MyInvocation.InvocationName -ne '.') in both PS1 wrappers.
8. Narrow dependency version upper-bounds or document why open ranges are intentional.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2.6M

PR Review — feat(skills): add tts-voiceover skill

The implementation is thoughtful and largely well-structured — the SSML injection fix, Entra ID token refresh logic, polyglot fuzz harness, and cross-platform wrappers all reflect careful engineering. However there are several required fixes before this can merge.

Issue Alignment

Linked issue #1417 — the changes in this PR appear aligned with adding a TTS voice-over skill using Azure Speech SDK. One concern: the hve-core-all.collection.yml changes promote ~10 unrelated artifacts from experimental to stable maturity, which goes well beyond the stated scope of this issue. See the inline comment on that file.

PR Template Compliance ⚠️

The Type of Change section is incomplete. The following boxes must be checked to match what was actually changed:

The current description only checks "Copilot skill," which is incomplete given the full scope of changes.

Coding Standards ⚠️

PowerShell scripts (Invoke-EmbedAudio.ps1, Invoke-GenerateVoiceover.ps1):

• Body of the if ($MyInvocation.InvocationName -ne '.') invocation guard is not indented — violates the standard structure from powershell-script.instructions.md.
• Missing try/catch block with explicit exit 0 / exit 1 codes in the main execution guard.
  See the inline comment on Invoke-EmbedAudio.ps1 line 60 for a corrected template (applies equally to Invoke-GenerateVoiceover.ps1).

SKILL.md — non-standard attribution footer (see inline comment on line 195).

Code Quality ✅

The Python code is clean:

• SSML injection is correctly mitigated: speaker notes are XML-escaped before acronym substitution (xml.sax.saxutils.escape), and quoteattr is used for voice/rate attributes.
• The polyglot fuzz harness correctly guards atheris-dependent code behind if __name__ == "__main__" and FUZZING: so pytest invocations stay clean.
• Token refresh logic for Entra ID auth is sound — the speech_resource_id is None short-circuit prevents NameError on speechsdk in dry-run mode.
• Bash wrappers follow the conventions (strict mode, proper quoting, main "$@" pattern, copyright headers).

The dependency-review.yml change is justified — certifi (MPL-2.0) and charset-normalizer (LGPL for detection code) are transitive dependencies of azure-* packages and the comments explain the rationale.

Scope Concerns ⚠️

Two changes in this PR are outside the stated scope and should either be documented or moved to separate PRs:

1. hve-core-all.collection.yml: ~10 unrelated artifacts are promoted from experimental to stable. See inline comment.
2. experimental.collection.yml: vscode-playwright skill is added to the collection without mention in the PR description. See inline comment.

Required Actions

1. ☐ Check GitHub Actions workflow under Type of Change in the PR description.
2. ☐ Check Script/automation under Type of Change in the PR description.
3. ☐ Either remove the unrelated maturity: experimental removals from hve-core-all.collection.yml and put them in a separate PR, or explicitly document and justify them in this PR.
4. ☐ Address the vscode-playwright addition to experimental.collection.yml — either document it or remove it.
5. ☐ Fix indentation and add try/catch + explicit exit codes in both PowerShell scripts (see inline comment on Invoke-EmbedAudio.ps1 line 60).
6. ☐ Remove the non-standard *🤖 Crafted with precision...* footer line from SKILL.md (see inline comment on line 195).

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2.7M

Review Summary

The tts-voiceover skill is well-structured overall: the Python implementation correctly escapes SSML injection vectors, the cross-platform wrapper pair (bash + PowerShell) is provided as required, the fuzz harness is present, and collection manifests are updated. However, there are PR template compliance gaps and several code quality issues that must be addressed before this is ready to merge.

Issue Alignment

Issue #1417 could not be read directly, but the PR description is detailed and coherent with the declared intent of adding an Azure Speech SDK TTS skill. No misalignment is apparent from the description alone.

PR Template Compliance

Required fixes:

• ❌ npm run lint:md-links is unchecked — marked "(deferred to CI)" but the PR template lists this as a required automated check that must pass before merging. Either run it locally and check the box, or link to a passing CI run that confirms zero broken links.

• ❌ "Script/automation" Type of Change checkbox not checked — the PR adds .py, .sh, and .ps1 files. The template's "Other" section includes * [ ] Script/automation (.ps1, .sh, .py), which should be checked alongside the Copilot skill checkbox.

• ⚠️ "Reviewed contribution with prompt-builder agent" removed from Type of Change — this item was present in the template's AI Artifacts section but is absent from the PR body's Type of Change section. It appears in the Checklist as unchecked with an N/A note; either restore it to the Type of Change section or confirm the N/A rationale applies there too.

Coding Standards

• ❌ SKILL.md attribution footer is not the last line — prompt-builder.instructions.md requires > Brought to you by microsoft/hve-core to be "the last line of body content." The *🤖 Crafted...* line appears after it. See inline comment on line 195.

• ❌ PowerShell guard block missing indentation — in both Invoke-GenerateVoiceover.ps1 and Invoke-EmbedAudio.ps1, the body of the if ($MyInvocation.InvocationName -ne '.') guard is not indented, inconsistent with repository PowerShell conventions. See inline comment on Invoke-GenerateVoiceover.ps1 line 83.

• ⚠️ [OutputType([void])] is not a valid PowerShell type — void is not a recognized .NET type identifier; use [System.Void] or omit the attribute entirely for functions that produce no pipeline output. See inline comment on TtsVoiceoverHelpers.psm1 line 50.

Code Quality

• ⚠️ Acronym key matching silently fails after XML escaping — safe_notes is XML-escaped before being passed to apply_acronym_aliases. Any custom lexicon key containing &, <, or > will never match because the text now holds &, <, >. The built-in defaults are unaffected, but the contract is undocumented and fragile for user-supplied lexicons. See inline comment on generate_voiceover.py line 73.

• 💡 speechsdk imported twice — azure.cognitiveservices.speech is imported inside both generate_audio() (line 105) and _run() (inside the if not args.dry_run: block). Python caches module imports so this is harmless, but the redundant import is unnecessary. Consider hoisting the import to the top of _run() and passing the module through, or importing lazily in one place only.

Action Items

1. Run npm run lint:md-links and mark the checkbox (or link a passing CI run).
2. Check the "Script/automation" box under Type of Change.
3. Remove the extra *🤖 Crafted...* line from SKILL.md (or move it above the standard footer).
4. Indent the body of the InvocationName guard block in both PowerShell wrapper scripts.
5. Fix [OutputType([void])] → [System.Void] or omit it in TtsVoiceoverHelpers.psm1.
6. Document the XML pre-escaping constraint in apply_acronym_aliases's docstring so future contributors understand the lexicon key restriction.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2M

Review Summary

This PR adds a well-structured tts-voiceover skill with cross-platform scripts, a fuzz harness, and proper collection registration. The overall approach is solid — SSML injection is addressed, token refresh logic is present, and the dry-run mode is a good usability touch. Two code correctness issues and two PR template gaps need to be resolved before merge.

Issue Alignment

The linked issue (#1417) could not be read (integrity filter), so full alignment cannot be confirmed. Based on the PR description, the implementation scope appears coherent with the stated feature: YAML speaker notes → SSML → WAV → PPTX embedding.

PR Template Compliance

Two Type of Change checkboxes are missing despite the corresponding files being present in the diff:

• "Script/automation (.ps1, .sh, .py)" — The PR adds generate_voiceover.py, embed_audio.py, fuzz_harness.py, two bash wrappers, and two PowerShell wrappers. This checkbox must be checked.
• "GitHub Actions workflow" — .github/workflows/dependency-review.yml is modified to allowlist new PyPI packages. This checkbox must be checked.

Please update the PR description to reflect these change types.

Code Quality Findings

🐛 Critical — AttributeError on empty content.yaml (inline comment on line 267–268)

yaml.safe_load() returns None for an empty file; calling .get() on None raises AttributeError and aborts the entire run. A yaml.YAMLError from malformed YAML is also unhandled. See the inline comment for a suggested per-slide try/except + isinstance guard.

⚠️ Convention — Deferred import inside generate_audio() (inline comment on line 112)

Per python-script.instructions.md, imports belong at module level. _make_entra_config correctly receives speechsdk as a parameter; generate_audio should follow the same pattern. See the inline comment for the suggested fix.

Coding Standards

• ✅ Copyright headers present on all Python, bash, and PowerShell files.
• ✅ set -euo pipefail on both bash wrappers.
• ✅ $ErrorActionPreference = 'Stop' on both PowerShell scripts.
• ✅ yaml.safe_load (not yaml.load) used throughout.
• ✅ xml.sax.saxutils.escape applied before acronym substitution — SSML injection is properly mitigated.
• ✅ pyproject.toml includes all required sections ([tool.ruff], [tool.pytest.ini_options], fuzz dependency group, fuzz_harness.py in python_files).
• ⚠️ lxml pinned as a direct dependency but not imported directly — see inline comment on pyproject.toml:8 for a clarification ask (non-blocking if intentional).

Action Items

1. ☑️ Check "Script/automation" and "GitHub Actions workflow" in the Type of Change section.
2. 🐛 Fix the AttributeError / YAMLError in the content.yaml parsing loop (line 267–268).
3. ⚠️ Move the azure.cognitiveservices.speech import out of generate_audio() by passing speechsdk as a parameter (line 112).
4. 💡 Add a comment to pyproject.toml explaining the lxml explicit pin, or remove it if it is purely transitive.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 2M

PR Review — feat(skills): add tts-voiceover skill

The implementation is well-structured: XML injection is correctly mitigated, Entra ID token refresh is implemented, the fuzz harness covers the right pure functions, and the cross-platform wrapper pattern (bash + PowerShell) is followed consistently. Two issues require changes before merge.

❌ Issue Alignment

Linked issue #1417 could not be read due to access restrictions. The PR description provides enough context to evaluate scope and intent.

⚠️ Scope creep detected — collections/experimental.collection.yml adds a vscode-playwright entry that is never mentioned in the PR description, Additional Notes, or commit history. This is an undisclosed change. Please remove it or explicitly document and justify it.

❌ PR Template Compliance

The Type of Change section is incomplete. The following checkboxes apply to this PR's actual changes and must be checked:

Checking only Copilot skill while omitting these applicable categories makes it harder for reviewers to quickly assess the change surface area.

✅ Coding Standards

Files are broadly compliant with the applicable instruction files:

• Python (python-script.instructions.md, python-test.instructions.md): Copyright headers ✅, SPDX identifiers ✅, pathlib.Path throughout ✅, create_parser() extracted ✅, proper exit codes ✅, logging (not print) for diagnostics ✅, fuzz harness with Atheris polyglot pattern ✅, ruff in dev deps ✅.
• PowerShell (powershell.instructions.md): #Requires -Version 7.0 ✅, [CmdletBinding()] ✅, $ErrorActionPreference = 'Stop' ✅, invocation guard ✅, comment-based help ✅, Export-ModuleMember ✅.
• Bash (bash.instructions.md): set -euo pipefail ✅, #!/usr/bin/env bash ✅, copyright headers ✅, main() pattern ✅, [[ ]] tests ✅, err() helper ✅.
• SKILL.md (prompt-builder.instructions.md): Required sections present in correct order ✅, attribution suffix in description: ✅, attribution footer ✅.

✅ Code Quality and Security

• XML injection: Notes are correctly escaped with xml.sax.saxutils.escape() before acronym substitution. Voice/rate attributes are protected with quoteattr(). ✅
• Credentials: Read exclusively from environment variables; no secrets in code. ✅
• Entra ID token refresh: token_expires_at - 300 guard is correct; the initial token is fetched before the loop so the token_expires_at = 0 default is never used in the auth comparison. ✅
• Path traversal: --lexicon reads a user-specified local file, which is expected CLI behavior. ✅
• Error handling: Both Python scripts handle KeyboardInterrupt, BrokenPipeError, and use proper exit codes. ✅
• Dependency review allowances (certifi MPL-2.0, charset-normalizer LGPL): Justification comments are accurate; these are transitive deps of azure-identity. ✅

💡 Non-Blocking Suggestions

See inline comments for:

1. Hardcoded xml:lang="en-US" in wrap_ssml — limits non-English voice support (informational for this experimental-maturity release).
2. Missing --region CLI flag — asymmetric with other configurable parameters.
3. Missing description in pyproject.toml — PEP 621 completeness.

🔧 Required Actions

1. Remove or explain the vscode-playwright entry in experimental.collection.yml.
2. Check GitHub Actions workflow and Script/automation in the Type of Change section.

tools:
  github:
    min-integrity: approved  # merged | approved | unapproved | none

Generated by PR Review for issue #1415 · ● 1.8M

Approving with some requested changes/additions.

Thanks, this is a really interesting skill in combination with the powerpoint one especially.
Leaving a few notes I recommend addressing, but approving overall, given you will close all Copilot other reviewer notes before merging.
Looking forward to trying it out!